[GPU] ASAN failure due to use of erased operation #116816
Conversation
If the target op is an scf.forall op which is erased by `rewriteOneForallCommonImpl` method it leads to asan failure since it's used later by `replaceUnitMappingIdsHelper` fn. Follows the same philosophy as done here: https://github.com/llvm/llvm-project/blob/aff98e4be05a1060e489ce62a88ee0ff365e571a/mlir/lib/Dialect/GPU/TransformOps/GPUTransformOps.cpp#L599 and later passed to the same method here: https://github.com/llvm/llvm-project/blob/aff98e4be05a1060e489ce62a88ee0ff365e571a/mlir/lib/Dialect/GPU/TransformOps/GPUTransformOps.cpp#L629
|
@llvm/pr-subscribers-mlir Author: Prashant Kumar (pashu123) ChangesIf the target op is an scf.forall op which is erased by Follows the same philosophy as done here: Full diff: https://github.com/llvm/llvm-project/pull/116816.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/GPU/TransformOps/GPUTransformOps.cpp b/mlir/lib/Dialect/GPU/TransformOps/GPUTransformOps.cpp
index 1528da914d546b..e836820c960339 100644
--- a/mlir/lib/Dialect/GPU/TransformOps/GPUTransformOps.cpp
+++ b/mlir/lib/Dialect/GPU/TransformOps/GPUTransformOps.cpp
@@ -853,6 +853,8 @@ DiagnosedSilenceableFailure mlir::transform::gpu::mapNestedForallToThreadsImpl(
"requires size-3 thread mapping");
}
+ Block *parentBlock = target->getBlock();
+
// Create an early zero index value for replacements.
Location loc = target->getLoc();
Value zero = rewriter.create<arith::ConstantIndexOp>(loc, 0);
@@ -872,7 +874,7 @@ DiagnosedSilenceableFailure mlir::transform::gpu::mapNestedForallToThreadsImpl(
// Replace ids of dimensions known to be 1 by 0 to simplify the IR.
// Here, the result of mapping determines the available mapping sizes.
- replaceUnitMappingIdsHelper<ThreadIdOp>(rewriter, loc, target, zero,
+ replaceUnitMappingIdsHelper<ThreadIdOp>(rewriter, loc, parentBlock, zero,
blockDims);
return DiagnosedSilenceableFailure::success();
|
|
@llvm/pr-subscribers-mlir-gpu Author: Prashant Kumar (pashu123) ChangesIf the target op is an scf.forall op which is erased by Follows the same philosophy as done here: Full diff: https://github.com/llvm/llvm-project/pull/116816.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/GPU/TransformOps/GPUTransformOps.cpp b/mlir/lib/Dialect/GPU/TransformOps/GPUTransformOps.cpp
index 1528da914d546b..e836820c960339 100644
--- a/mlir/lib/Dialect/GPU/TransformOps/GPUTransformOps.cpp
+++ b/mlir/lib/Dialect/GPU/TransformOps/GPUTransformOps.cpp
@@ -853,6 +853,8 @@ DiagnosedSilenceableFailure mlir::transform::gpu::mapNestedForallToThreadsImpl(
"requires size-3 thread mapping");
}
+ Block *parentBlock = target->getBlock();
+
// Create an early zero index value for replacements.
Location loc = target->getLoc();
Value zero = rewriter.create<arith::ConstantIndexOp>(loc, 0);
@@ -872,7 +874,7 @@ DiagnosedSilenceableFailure mlir::transform::gpu::mapNestedForallToThreadsImpl(
// Replace ids of dimensions known to be 1 by 0 to simplify the IR.
// Here, the result of mapping determines the available mapping sizes.
- replaceUnitMappingIdsHelper<ThreadIdOp>(rewriter, loc, target, zero,
+ replaceUnitMappingIdsHelper<ThreadIdOp>(rewriter, loc, parentBlock, zero,
blockDims);
return DiagnosedSilenceableFailure::success();
|
|
Is there an existing tests failing with ASAN? |
Tests failed in the downstream project, but I'll add them here. |
I'm curious if we need to add tests for ASAN failures because I believe this patch addresses a fundamental issue. |
|
This is a problem of test coverage. |
|
@joker-eph @ftynse Do you know how to add tests for this? transform.gpu.map_nested_forall_to_threads always expect a gpu.launch op. The problem happens when we directly use
without the check.
|
ftynse
left a comment
ftynse
left a comment
There was a problem hiding this comment.
Take the downstream tests that triggered the asan issue, minimize it by removing downstream-specific parts, and put it somewhere it can run. Ideally, it would be a "unit test", i.e, a .mlir file running one pass and checking the output. Many MLIR integrators are running all tests under asan and would have seen the failure had it had the corresponding test.
Regardless of that, the change itself looks off to me. It calls replaceUnitMappingIdsHelper on the parent block of a target, which breaks the fundamental assumption of the transform dialect (and passes by the way, imaging target is the root operation of the interpreter pass).
|
The problem was with the downstream calling convention. I am closing this. |
4 participants
If the target op is an scf.forall op which is erased by
rewriteOneForallCommonImplmethod, it leads to an ASAN failure since it's used later byreplaceUnitMappingIdsHelperfn.Follows the same philosophy as done here:
llvm-project/mlir/lib/Dialect/GPU/TransformOps/GPUTransformOps.cpp
Line 599 in aff98e4
llvm-project/mlir/lib/Dialect/GPU/TransformOps/GPUTransformOps.cpp
Line 629 in aff98e4